又更新了，但不知道更了什么#13
Conversation
There was a problem hiding this comment.
Pull request overview
This PR primarily focuses on code documentation improvements and refactoring of the line-clearing animation system in a Tetris FPGA implementation written in SystemVerilog. The PR title "又更新了，但不知道更了什么" (Updated again, but don't know what was updated) is somewhat ironic given that the changes involve significant documentation enhancements and animation logic restructuring.
Key changes include:
- Standardized all comments to Chinese for consistency across the codebase
- Refactored the line-clearing animation from an in-place effect to a fragment-based particle system that allows blocks to fly outside the game grid
- Cleaned up excessive whitespace and comment decorations throughout the code
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
tetris_logic.sv |
Added documentation comments and cleaned up whitespace; removed decorative "---" from comment markers |
lfsr_galois_3bit.sv |
Translated English comments to Chinese for consistency; clarified LFSR implementation details |
block_renderer.sv |
Added inline comments; completely refactored line-clearing animation from in-place cracking to fragment scatter effect; corrected game-over text rendering comment; added excessive blank lines |
dfx_runtime.txt |
Added Xilinx DFX runtime report file (build artifact) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (int src_y = 0; src_y < 20; src_y++) begin | ||
| for (int src_x = 0; src_x < 10; src_x++) begin | ||
| if (!fragment_drawn && clearing_rows[src_y]) begin | ||
| logic [3:0] src_color; | ||
| logic [1:0] src_dir; | ||
| logic src_randbit; | ||
| logic signed [10:0] pixel_offset_x, pixel_offset_y; | ||
| logic [5:0] local_x, local_y; | ||
| logic signed [10:0] src_screen_x, src_screen_y; | ||
| logic signed [10:0] fragment_screen_x, fragment_screen_y; | ||
|
|
||
| src_color = game_grid_array[src_y][src_x]; | ||
|
|
||
| if (src_color != 4'h0) begin // 非空方块 | ||
| // 计算源方块的飞散方向 | ||
| src_dir = {src_x[0]^src_y[1], src_x[1]^src_y[0]}; | ||
| src_randbit = src_x[2]^src_y[2]^src_x[0]; | ||
|
|
||
| // 计算源方块在屏幕上的左上角位置 | ||
| src_screen_x = GRID_START_X + src_x * BLOCK_SIZE; | ||
| src_screen_y = src_y * BLOCK_SIZE; | ||
|
|
||
| // 计算飞散后的位置 | ||
| fragment_screen_x = src_screen_x; | ||
| fragment_screen_y = src_screen_y; | ||
|
|
||
| case (src_dir) | ||
| 2'b00: fragment_screen_x = src_screen_x + anim_disp; // 向右飞 | ||
| 2'b01: fragment_screen_x = src_screen_x - anim_disp; // 向左飞 | ||
| 2'b10: fragment_screen_y = src_screen_y - anim_disp; // 向上飞 | ||
| default: fragment_screen_y = src_screen_y + anim_disp; // 向下飞 | ||
| endcase | ||
|
|
||
| // 计算当前像素相对于飞散后位置的偏移 | ||
| pixel_offset_x = $signed(curr_pix_x) - fragment_screen_x; | ||
| pixel_offset_y = $signed(curr_pix_y) - fragment_screen_y; | ||
|
|
||
| // 检查是否在碎片范围内 | ||
| if (pixel_offset_x >= 0 && pixel_offset_x < BLOCK_SIZE && pixel_offset_y >= 0 && pixel_offset_y < BLOCK_SIZE) begin | ||
| local_x = pixel_offset_x[5:0]; | ||
| local_y = pixel_offset_y[5:0]; | ||
|
|
||
| // 碎片形状与稀疏门控（随时间更稀疏） | ||
| pattern = (local_x[2] ^ local_y[1] ^ src_randbit) | (local_x[1] & ~local_y[2]); | ||
| dist_bucket = (src_dir[0]) ? local_x[4:3] : local_y[4:3]; | ||
| gate = (dist_bucket >= anim_phase[4:3]); | ||
|
|
||
| if (pattern && gate) begin | ||
| pixel_color = ~pixel_color; // 碎片颜色取反,更加醒目 | ||
| fragment_drawn = 1; | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
This nested loop structure iterates up to 200 times (20 rows × 10 columns) for every pixel being rendered, which is extremely inefficient. For a 640×480 display, this means up to 61.44 million loop iterations per frame. This will likely cause severe synthesis issues and timing violations.
Consider refactoring to:
- Pre-calculate fragment positions in a sequential clocked process
- Store active fragments in a small array (one entry per clearing row)
- Check only the relevant fragments for the current pixel instead of iterating through all grid positions
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
[nitpick] Consider removing these excessive blank lines (20+ empty lines). While whitespace can improve readability, this many consecutive blank lines doesn't add value and increases file size unnecessarily.
| logic [4:0] anim_phase; // 动画阶段 0-31 | ||
| logic [5:0] anim_disp; // 碎片位移 | ||
| logic fragment_drawn; // 是否已绘制碎片 |
There was a problem hiding this comment.
These local variable declarations within the always_comb block, combined with the nested loops below, create highly complex combinational logic. This can lead to:
- Long synthesis times
- Potential timing closure issues
- Difficulty in static timing analysis
Consider moving these to module-level declarations (like the existing phase, crack_t, disp variables at lines 39-48) or refactoring the animation logic to use sequential (clocked) logic instead of pure combinational logic.
| // 遍历所有可能的源方块位置（游戏网格内） | ||
| for (int src_y = 0; src_y < 20; src_y++) begin | ||
| for (int src_x = 0; src_x < 10; src_x++) begin | ||
| if (!fragment_drawn && clearing_rows[src_y]) begin | ||
| logic [3:0] src_color; | ||
| logic [1:0] src_dir; | ||
| logic src_randbit; | ||
| logic signed [10:0] pixel_offset_x, pixel_offset_y; | ||
| logic [5:0] local_x, local_y; | ||
| logic signed [10:0] src_screen_x, src_screen_y; | ||
| logic signed [10:0] fragment_screen_x, fragment_screen_y; |
There was a problem hiding this comment.
Declaring multiple logic variables inside deeply nested loops within an always_comb block creates extremely complex combinational logic. Since these declarations are inside loops that run 200 iterations per pixel, the synthesis tool must handle thousands of temporary signals.
Consider:
- Moving these declarations to module-level scope
- Restructuring to avoid nested loops in combinational logic
- Using a clocked process to compute fragment positions once per frame rather than per pixel
| // 遍历所有可能的源方块位置（游戏网格内） | |
| for (int src_y = 0; src_y < 20; src_y++) begin | |
| for (int src_x = 0; src_x < 10; src_x++) begin | |
| if (!fragment_drawn && clearing_rows[src_y]) begin | |
| logic [3:0] src_color; | |
| logic [1:0] src_dir; | |
| logic src_randbit; | |
| logic signed [10:0] pixel_offset_x, pixel_offset_y; | |
| logic [5:0] local_x, local_y; | |
| logic signed [10:0] src_screen_x, src_screen_y; | |
| logic signed [10:0] fragment_screen_x, fragment_screen_y; | |
| // Declare temporary variables once at the top of the always_comb block to avoid excessive combinational logic | |
| logic [3:0] src_color; | |
| logic [1:0] src_dir; | |
| logic src_randbit; | |
| logic signed [10:0] pixel_offset_x, pixel_offset_y; | |
| logic [5:0] local_x, local_y; | |
| logic signed [10:0] src_screen_x, src_screen_y; | |
| logic signed [10:0] fragment_screen_x, fragment_screen_y; | |
| // 遍历所有可能的源方块位置（游戏网格内） | |
| for (int src_y = 0; src_y < 20; src_y++) begin | |
| for (int src_x = 0; src_x < 10; src_x++) begin | |
| if (!fragment_drawn && clearing_rows[src_y]) begin |
| DFXRuntime Profile Report: | ||
| Total Application(DFX) Runtime : CPU : 0:2:18 WALL : 0:0:0 0.00 % |
There was a problem hiding this comment.
This appears to be a Xilinx DFX (Dynamic Function eXchange) runtime report file, which is typically an auto-generated output file. Build artifacts and tool-generated reports should generally not be tracked in version control.
Consider adding dfx_runtime.txt or *.txt (if appropriate) to the .gitignore file to prevent committing build outputs.
No description provided.